Use proper HTTP client for fetching credentials #2041
Conversation
ramondeklein
force-pushed
the
http-client-fix
branch
3 times, most recently
from
f1d5c43 to
ed02e8d
Compare
ramondeklein
force-pushed
the
http-client-fix
branch
from
ed02e8d to
2be0ccc
Compare
| var retryable bool // Indicates if request can be retried. | ||
| var bodySeeker io.Seeker // Extracted seeker from io.Reader. | ||
| var reqRetry = c.maxRetries // Indicates how many times we can retry the request | ||
| var retryable bool // Indicates if request can be retried. | ||
| var bodySeeker io.Seeker // Extracted seeker from io.Reader. | ||
| reqRetry := c.maxRetries // Indicates how many times we can retry the request |
There was a problem hiding this comment.
This was changed by my IDE to follow coding guidelines.
| trCopy := tr.Clone() | ||
| trCopy.TLSClientConfig.Certificates = []tls.Certificate{i.Certificate} |
There was a problem hiding this comment.
This was the best alternative that I could come up with. We don't want to change the TLSClientConfig of the original HTTP transport, because that may have security and parallelization issues.
ramondeklein
force-pushed
the
http-client-fix
branch
2 times, most recently
from
c1c6c07 to
f9f409f
Compare
| if i.Client == nil { | ||
| i.Client = &http.Client{} | ||
| } |
There was a problem hiding this comment.
Previously, the i.Client was set when NewSTSCertificateIdentity created the STSCertificateIdentity structure. So we need to check for nil here, because the default HTTP client is now nil.
ramondeklein
force-pushed
the
http-client-fix
branch
from
f9f409f to
57332bd
Compare
| // Clone the HTTP client (patch the HTTP transport) | ||
| clientCopy := *client | ||
| clientCopy.Transport = trCopy |
There was a problem hiding this comment.
Although not the most elegant solution, I guess this is the safest method to ensure we get a copy of the HTTP client.
|
@harshavardhana I think I addressed your comments. If you prefer, I could add some unit-tests to ensure that the custom transport is being invoked. Not sure if that's needed. |
|
@ramondeklein I though this code was on the "Unesco World Heritage List" - nobody has been touching it for years :) LGTM :) |
This was broken in #2041
This PR ensures that the HTTP client that is set on the
MinioClientwill also be used when the client needs to fetch credentials via an HTTP request.CredContextstructure that abstracts the HTTP client and may also be used to add more context (when needed).STSCertificateIdentitymethod passes certificates via theTLSClientConfig, so in this case the HTTP transport is cloned and it sets the certificate explicitly. The original implementation did set additionalhttp.Transportfields, but this has been removed.credentials.(*Credentials).Get()function is deprecated andProvider.GetWithCredContext()should be used instead. It's still there to provide backward compatibility.madminto also pass the custom HTTP client.Fixes #2040.